fix(InfoTip): wrap focus #3206
fix(InfoTip): wrap focus #3206
Conversation
|
View your CI Pipeline Execution ↗ for commit e078377 ☁️ Nx Cloud last updated this comment at |
aresnik11
left a comment
aresnik11
left a comment
There was a problem hiding this comment.
Thanks for bringing this over the finish line!
There was a problem hiding this comment.
Nice changes to address the edge cases!
Left some thoughts, LMK if these would've been addressed in another PR, because none of my comments should be "blocking" per se. But maybe a "blocking" comment is with the "focus forward" behavior, I did have a question about the example, where the tabbing wasn't "contained" to the infotip after tabbing back to the infotip button.
| const getFocusableElements = useCallback(() => { | ||
| const popoverContent = popoverContentNodeRef.current; | ||
| if (!popoverContent) return []; | ||
|
|
||
| return Array.from( | ||
| popoverContent.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR) | ||
| ); | ||
| }, []); | ||
|
|
||
| const clearAndSetTimeout = useCallback( |
There was a problem hiding this comment.
Non-blocking: I wonder if this code/logic can be in utils?
seems like some external teams "need" more control over focus management, so maybe having it live in a more general file would help in the long run.
There was a problem hiding this comment.
Another thought of how eventlisteners, or other helper functions can be abstracted elsewhere (and hopefully also re-used)
There was a problem hiding this comment.
great idea - will move!
There was a problem hiding this comment.
love it, it is done
|
|
||
| /* | ||
| * For floating placement, screenreader text comes before button to maintain | ||
| * correct DOM order despite Portal rendering. See GM-797 for planned fix. |
There was a problem hiding this comment.
I believe this is now GMT-64
see: https://skillsoftdev.atlassian.net/browse/GMT-64
|
|
||
| ### Floating Placement | ||
|
|
||
| When using `placement="floating"`, InfoTips implement **forward focus wrapping** to keep users within the tip context: |
There was a problem hiding this comment.
would be good to define what forward focus wrapping is
There was a problem hiding this comment.
I thought it meant like it'd be specific to focusing within the infotip link/buttons and back to the button itself and keep some kinda focus trap there.
but now I'm unsure because after we tab back to the button, then tabbing doesn't go back into the popover of the infotip.
There was a problem hiding this comment.
yeah i removed the focus trap so there is more natural tabbing, i'll reword!
| <Canvas of={InfoTipStories.Placement} /> | ||
|
|
||
| ## InfoTips with links or buttons | ||
| ## Keyboard Navigation & Accessibility |
There was a problem hiding this comment.
| ## Keyboard Navigation & Accessibility | |
| ## Keyboard navigation & accessibility |
Headings should be sentence case
|
|
||
| <Canvas of={InfoTipStories.WithLinksOrButtons} /> | ||
|
|
||
| ### Floating Placement |
There was a problem hiding this comment.
| ### Floating Placement | |
| ### Floating placement |
| {...args} | ||
| info={ | ||
| <Text> | ||
| <Text ref={ref} tabIndex={-1}> |
There was a problem hiding this comment.
Does this mean that the whole text gets focus when the infotip is clicked?
If so, it's a bit hard to see outright — maybe the 1st anchor can be the one that has focus? or the 2nd one to showcase that this programmatic focus is intentional?
And then the text can also change where it's like
"This info tip has a link but focus on clicking actually is set to 2nd link! Use the onClick prop!
There was a problem hiding this comment.
yeah this is a change to focusing the container instead of the links first which is implemented in this PRhttps://github.com//pull/3215 - if you turn on VO you'll see the virtual cursor wrapping the entire text instead of the first link/button
|
|
||
| <Box mt={16}> | ||
| <Text fontSize={14}> | ||
| <strong>Test Escape Key Behavior with Modals:</strong> |
There was a problem hiding this comment.
| <strong>Test Escape Key Behavior with Modals:</strong> | |
| <strong>Test Escape Key Behavior with Modals: </strong> |
There was a problem hiding this comment.
^ cause there's no gap between this sentence and the next
| </Text> | ||
| <Box as="ul" fontSize={14} pl={16}> | ||
| <li> | ||
| <strong>Floating - Tab:</strong> Navigates forward through links, |
There was a problem hiding this comment.
curious: Is this how we'd recommend making bold text?
There was a problem hiding this comment.
if don't need any other styles just using the html semantics is perfectly viable
| 1. Click the InfoTip to open it | ||
| <br /> | ||
| 2. Click "Open Modal" button | ||
| <br /> | ||
| 3. Press Escape - should close modal only (InfoTip stays open) | ||
| <br /> | ||
| 4. Press Escape again - should close InfoTip | ||
| <br /> | ||
| <br /> | ||
| <em> | ||
| InfoTip detects when modals are open and defers Escape key | ||
| handling to them. | ||
| </em> |
There was a problem hiding this comment.
This can prob be simplified to a <ol> instead (without the numbering and the br
| title="Test Modal" | ||
| onRequestClose={() => setIsModalOpen(false)} | ||
| > | ||
| <Box p={16}> |
There was a problem hiding this comment.
small nit: FlexBox seems cleaner to me cause of the placement of the FillButton
|
📬 Published Alpha Packages: |
|
🚀 Styleguide deploy preview ready! Preview URL: https://6920d8badf1150be0b1bcd56--gamut-preview.netlify.app |
Overview
Wraps focus when tabbing out of InfoTips for both inline + floating variants, extended
@aresnik11 wrote the functional code for the focus wrap, i just DRYed it up, added the tests + sanitized the screenreader text and made a few tweaks to global esc on inline tips as well
i also noticed the width was wrong on floating -right infotips, so i've fixed that. I also changed the examples to containerRefs since I'm going to change the onClick behavior in the following PR. The status in the InfoTip story has been updated to reflect that.
PR Checklist
Testing Instructions
KeyboardNavigationstoryPR Links and Envs